-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove parser transform for nil? in macro expressions #10616
Remove parser transform for nil? in macro expressions #10616
Conversation
What if we handle that |
"Calling this method will always return false" implies this method actually exist. But in the macro language, it doesn't need to exist at all. |
What will be the value of this? {% puts nil.nil? %} |
It doesn't compile. |
Oh, I thought this "Now" meant "now before this PR", but it seems to be "in this PR". I think this is fine then 👍 |
Just note that if we ever decide to be able to add custom methods to AST nodes in macros, |
@asterite I'd suggest not having EDIT: Forgot there's actually |
Oh, yeah, that makes sense. I guess for now we can go with this, and later on change it if needed, still in a backwards compatible way. |
While trivial to fix and the |
I would consider the fact that this code compiled as a compiler bug. It should've always been an error. Fixing bugs can obviously break code that relied on the bug behaviour, but that shouldn't prevent fixing bugs, right? 😄 |
In my mind calling |
Yeah, maybe we should add that. It's not strictly necessary though. And I'm not sure about combining |
For example, when you do |
#10616 removed a bug that the compiler accepted calls to nil? in macro expressions where that method does not exist. These calls were internally transformed to is_a?(Nil). In the macro interpreter, no value can be of type Nil because that's a runtime type, so any nil? call would always return false. Removing that transformation from the parser made nil? calls in macro expressions errors because that method is not defined. But users would expect nil.nil? to return true. So this patch implements ASTNode#nil? in the macro interpreter. It returns true if the value is a NilLiteral or Nop. This is technically a feature addition, but we should still merge it for 1.1 despite the feature freeze. It actually avoids a breaking change that would occur with #10616 alone. Code that worked in 1.0 would no longer compile (even though it was already broken before and just worked by chance). With this PR, it continues to compile and fulfills the intended purpose.
Resolves #10534